frontend/src/components/EmptyStates/EmptyAlbumsState.tsx (1)

3-28: Consider adding aria-hidden to decorative icons.

The component structure and implementation are excellent. The layout, styling, dark mode support, and user guidance are well-executed.

For enhanced accessibility, consider adding aria-hidden="true" to decorative icons on lines 7, 18, and 22, since they don't convey information beyond what the text already provides.

Example for line 7:

-        <BookImage className="h-16 w-16 text-gray-400" strokeWidth={1.5} />
+        <BookImage className="h-16 w-16 text-gray-400" strokeWidth={1.5} aria-hidden="true" />

frontend/src/pages/Album/AlbumDetail.tsx (2)

153-174: Backend data transformation handles schema variations defensively.

The transformation logic safely handles multiple possible backend response structures using optional chaining and defaults. However, the use of any types suggests the backend API contract could benefit from stronger typing.

Consider defining a TypeScript interface for the backend album response to eliminate the any casts and improve type safety:

interface BackendAlbum {
  album_id: string;
  album_name: string;
  description?: string;
  is_locked?: boolean;
  cover_image_path?: string;
  image_count?: number;
  created_at?: string;
  updated_at?: string;
}

---

176-213: Image loading works but has inefficient data fetching pattern.

The implementation fetches all images from the library, then filters by album image IDs. While functional, this becomes inefficient as the image library grows.

Consider updating the backend getAlbumImages endpoint to return full image objects rather than just IDs, eliminating the need to fetch and filter the entire image library:

// Instead of fetching all images
const { data: allImagesData } = usePictoQuery({
  queryKey: ['images'],
  queryFn: () => fetchAllImages(),
  enabled: !!albumId && !!album,
});

// Backend could return full image objects directly
const { data: imagesData } = usePictoQuery({
  queryKey: ['album-images', albumId],
  queryFn: () => getAlbumImages(albumId!, password ? { password } : undefined),
  enabled: !!albumId && !!album,
});

frontend/src/features/albumsSlice.ts (1)

56-70: Consider syncing image_count from server.

The local image_count updates (increment on add, decrement on remove) are correct, but could drift from the server if operations fail or are retried. Consider refreshing the selected album from the server after image operations complete to ensure consistency.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

backend/tests/test_albums.py (1)

149-185: Consider renaming test function for consistency.

The test function name still contains "hidden" (test_get_all_albums_include_hidden), while the docstring and implementation have been updated to use "locked" terminology. For consistency, consider renaming this to test_get_all_albums_include_locked.

-    def test_get_all_albums_include_hidden(self, mock_db_album, mock_db_locked_album):
+    def test_get_all_albums_include_locked(self, mock_db_album, mock_db_locked_album):

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

backend/app/database/albums.py (1)

107-118: LGTM! Past issue with unused parameter resolved.

The removal of the show_hidden/show_locked parameter addresses the previous review comment about an ignored parameter. The function now has clear, unambiguous behavior—it always returns all albums.

backend/app/routes/albums.py (2)

128-182: LGTM! Update endpoint correctly refactored.

The endpoint correctly uses is_locked throughout, and there's no duplicate route decorator issue (which was mentioned in a previous review). The password validation logic for locked albums is properly maintained.

---

376-430: LGTM! Cover image endpoint is well-implemented.

The new endpoint has comprehensive validation:

1. Album existence check
2. Image membership verification (image must be in the album)
3. Image existence validation

The use of db_get_image_path maintains proper architectural separation (addressing a previous review comment about inline database queries).

frontend/src/components/Albums/AddImagesToAlbumDialog.tsx (2)

34-44: Remove redundant state duplication.

The allImages state (line 32) unnecessarily duplicates imagesData.data. This adds complexity, extra re-renders, and a type assertion that could be unsafe.

Apply this diff to use imagesData.data directly:

-  const [allImages, setAllImages] = useState<Image[]>([]);

   const { data: imagesData, isLoading } = usePictoQuery({
     queryKey: ['images'],
     queryFn: () => fetchAllImages(),
     enabled: isOpen,
   });

-  useEffect(() => {
-    if (imagesData?.data) {
-      setAllImages(imagesData.data as Image[]);
-    }
-  }, [imagesData]);

Then update line 104 to use imagesData?.data directly:

-  const filteredImages = allImages.filter((image) => {
+  const filteredImages = (imagesData?.data as Image[] || []).filter((image) => {

---

46-96: Simplify by removing manual loader dispatches.

The component uses both manual loader dispatches (showLoader/hideLoader at lines 50, 61, 94) and the isPending flag from the mutation. This is redundant—isPending already provides the loading state needed for UI feedback.

Remove the manual loader dispatches and rely solely on isPending:

   const handleSubmit = () => {
     if (selectedImages.size === 0) {
       dispatch(
         showInfoDialog({
           title: 'No Images Selected',
           message: 'Please select at least one image to add to the album.',
           variant: 'info',
         }),
       );
       return;
     }

-    dispatch(showLoader('Adding images to album...'));
     addImagesMutate({ image_ids: Array.from(selectedImages) });
   };

And remove hideLoader calls from the mutation callbacks (lines 50, 61):

   const { mutate: addImagesMutate, isPending } = usePictoMutation({
     mutationFn: (data: { image_ids: string[] }) =>
       addImagesToAlbum(albumId, data),
     onSuccess: () => {
-      dispatch(hideLoader());
       dispatch(
         showInfoDialog({
           title: 'Success',
           message: `Added ${selectedImages.size} image${selectedImages.size > 1 ? 's' : ''} to album!`,
           variant: 'info',
         }),
       );
       handleClose();
     },
     onError: (error: any) => {
-      dispatch(hideLoader());
       dispatch(
         showInfoDialog({
           title: 'Error',
           message: error?.message || 'Failed to add images. Please try again.',
           variant: 'error',
         }),
       );
     },
   });

The button states at lines 200 and 206 already use isPending correctly.

backend/app/database/albums.py (1)

220-322: Consider using the context manager consistently.

While not critical, there's an inconsistency in connection handling: db_update_album_cover_image and db_get_image_path use manual connection management, while some other functions (e.g., db_delete_album, db_add_images_to_album) use the get_db_connection() context manager defined at the top of the file. The context manager provides retry logic and better error handling.

Consider refactoring these functions to use the context manager:

 def db_update_album_cover_image(album_id: str, cover_image_path: str):
     """Update the cover image path for an album"""
-    conn = sqlite3.connect(DATABASE_PATH)
-    cursor = conn.cursor()
-    try:
+    with get_db_connection() as conn:
+        cursor = conn.cursor()
         cursor.execute(
             "UPDATE albums SET cover_image_path = ? WHERE album_id = ?",
             (cover_image_path, album_id),
         )
-        conn.commit()
-    finally:
-        conn.close()

Apply similar changes to db_get_image_path and other functions using manual connection management.

backend/app/routes/albums.py (1)

43-45: Consider optimizing image count queries.

The current implementation performs N+1 queries: one to fetch all albums, then one per album to count images. For large album collections, this could be slow.

Consider adding a database function that returns albums with image counts in a single query:

def db_get_all_albums_with_counts():
    """Get all albums with their image counts in a single query"""
    conn = sqlite3.connect(DATABASE_PATH)
    cursor = conn.cursor()
    try:
        cursor.execute(
            """
            SELECT a.album_id, a.album_name, a.description, a.is_locked, 
                   a.password_hash, a.cover_image_path, COUNT(ai.image_id) as image_count
            FROM albums a
            LEFT JOIN album_images ai ON a.album_id = ai.album_id
            GROUP BY a.album_id
            """
        )
        return cursor.fetchall()
    finally:
        conn.close()

This reduces the operation from O(n) queries to O(1).

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

frontend/src/components/Albums/EditAlbumDialog.tsx (1)

38-40: Prefer explicit typing over any for type safety.

The mutation function parameter data is typed as any, which bypasses TypeScript's type checking. Consider using the UpdateAlbumRequest type instead.

Apply this diff to improve type safety:

-  const { mutate: updateAlbumMutate, isPending } = usePictoMutation({
-    mutationFn: ({ albumId, data }: { albumId: string; data: any }) =>
-      updateAlbum(albumId, data),
+  const { mutate: updateAlbumMutate, isPending } = usePictoMutation({
+    mutationFn: ({ albumId, data }: { albumId: string; data: UpdateAlbumRequest }) =>
+      updateAlbum(albumId, data),

You'll also need to import the type:

import { UpdateAlbumRequest } from '@/types/Album';

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

frontend/src/pages/Album/AlbumDetail.tsx (4)

131-152: Extensive as any casting bypasses type safety for backend data transformation.

Consider defining a typed interface for the backend album response shape (e.g., BackendAlbumResponse) and using it in place of the double as any cast. This would catch field renames or missing properties at compile time rather than silently producing undefined.

---

168-178: O(n×m) image filtering — use a Set for the lookup.

imageIds.includes(img.id) inside allImages.filter(...) is O(n×m). For large libraries this degrades quickly. Convert imageIds to a Set first.

Also, fetching the entire image library (fetchAllImages) just to hydrate album images is expensive. Consider a backend endpoint that returns full image objects for an album directly.

Quick fix for the Set lookup

-      const albumImages = allImages.filter((img) => imageIds.includes(img.id));
+      const imageIdSet = new Set(imageIds);
+      const albumImages = allImages.filter((img) => imageIdSet.has(img.id));

---

62-90: Sequential waterfall: album → images & allImages queries are chained via enabled: !!album.

Both the album-images and all-images queries depend on album being set in Redux (via the first useEffect), which only runs after the album query resolves. This creates a request waterfall (album fetch → render → effect → re-render → images fetch). If you already have albumId from the URL, you could enable the images queries with just !!albumId, removing the dependency on album being fully hydrated in Redux.

---

144-147: Fallback timestamps (new Date().toISOString()) are misleading.

If the backend doesn't return created_at/updated_at, fabricating a current timestamp silently hides missing data and may cause incorrect sorting or display. Consider using null / undefined with an appropriate type to surface the absence.

frontend/src/pages/Album/Album.tsx (1)

88-103: Loader persists during initial render when albums is empty.

While the useEffect dispatches showLoader during isLoading, the component body still renders. If albums in Redux starts as [], <EmptyAlbumsState /> briefly flashes behind the loader overlay. Consider a guard (e.g., if (isLoading) return null;) or checking isLoading before rendering the album grid.

frontend/src/components/Albums/AddImagesToAlbumDialog.tsx (1)

154-158: prefers-color-scheme may not match the app's active theme.

If the app uses its own theme toggle (e.g., a dark/light class on <html>), window.matchMedia('(prefers-color-scheme: dark)') reflects the OS preference, not the app state. Consider checking the same source of truth the rest of the app uses (e.g., a dark class on document.documentElement).